-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
Use Matrix Project Plugin in Security914Test, not Credentials Plugin #26090
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Fixes jenkinsci#26088 The user interface update to the Credentials plugin in pull request: * jenkinsci/credentials-plugin#990 removed the image that this test used for its initial configuration. The change was released in Credentials plugin: * https://github.com/jenkinsci/credentials-plugin/releases/tag/1460.v48765a_c7d849 The new release of the Credentials plugin was included in Jenkins core with pull request: * jenkinsci#26046 This change switches from the Credentials plugin to the Matrix Project plugin as the base for the test. The assumption is that user interface changes are less likely to the Matrix Project plugin. Testing done: * Confirmed that the tests fail on Windows without this change * Confirmed that the tests pass on Windows with this change
|
Test failure is visible on the master branch. Other merges to the master branch are blocked by the test failures. Let's merge this one as soon as we see that it passes tests. |
Wadeck
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not manually tested but your rationale and changes seem to be good enough 👍 Thanks Mark!
daniel-beck
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
|
How come we have a release if this has been failing since 4120c28? Why was the PR even merged with security test failures? Is this a Smart Tests issue? |
The test only fails on Windows. Release builds are run on Linux. The test with the pull request change passes on my Linux computer when I remove the
I have not investigated, but I suspect that Smart Tests did not run the test on the pull request build. The test has been passing for a very long time, so it would have very little reason to choose to run it. If that is correct, then this is only the second time we've had a failure arrive on the master branch due to Smart Tests selecting tests. I think the cost savings are worth a few failures in the long time we've had Smart Tests enabled. Smart Tests seems to have skipped that test on the pull request build of: The GitHub checks for that pull request show that 890 tests were run on Windows JDK 25, while 25325 tests were run Linux JDK 25 |
Use Matrix Project Plugin in Security914Test, not Credentials Plugin
Fixes issue:
The user interface update to the Credentials plugin in pull request:
removed the image that this test used for its initial configuration.
The change was released in Credentials plugin:
The new release of the Credentials plugin was included in Jenkins core 2.545 with pull request:
This change switches from the Credentials plugin to the Matrix Project plugin as the base for the test. The assumption is that user interface changes are less likely to the Matrix Project plugin.
Testing done
Test also passes on Linux, but I did not want to change the platform assumption for the test.
Proposed changelog entries
Proposed changelog category
/label skip-changelog
Proposed upgrade guidelines
N/A
Submitter checklist
New public classes, fields, and methods are annotated with@Restrictedor have@since TODOJavadocs, as appropriate.New deprecations are annotated with@Deprecated(since = "TODO")or@Deprecated(forRemoval = true, since = "TODO"), if applicable.UI changes do not introduce regressions when enforcing the current default rules of Content Security Policy Plugin. In particular, new or substantially changed JavaScript is not defined inline and does not callevalto ease future introduction of Content Security Policy (CSP) directives (see documentation).For dependency updates, there are links to external changelogs and, if possible, full differentials.For new APIs and extension points, there is a link to at least one consumer.Desired reviewers
@Wadeck
Before the changes are marked as
ready-for-merge:Maintainer checklist
upgrade-guide-neededlabel is set and there is a Proposed upgrade guidelines section in the pull request title (see example).lts-candidateto be considered.